Make entity fields required by default #81
Conversation
There was a problem hiding this comment.
Thanks a lot for this contribution, @olivier-thatch! The changes look generally good, and I appreciate that you not only highlighted this problem, but also suggested a solution. I do have some thoughts on a potential simplification in one of the methods, which I've detailed in a comment below.
olivier-thatch
force-pushed
the
required-by-default
branch
from
0a992e3 to
14455d2
Compare
olivier-thatch
force-pushed
the
required-by-default
branch
from
14455d2 to
e017856
Compare
olivier-thatch
force-pushed
the
required-by-default
branch
3 times, most recently
from
07a3f3b to
374be93
Compare
|
I've updated the PR to account for another case of conditional exposure: |
olivier-thatch
force-pushed
the
required-by-default
branch
from
374be93 to
1bdd95f
Compare
|
Hi @numbata, any chance this could get merged and released sometimes soon? |
|
I'll merge this, open an issue for a release, can do if we don't see @numbata for a while. |
|
I'll do release tomorrow (if that's okay), but I think we should add a bit words into the CHANGELOG or README, as this PR changes existing behavior. |
|
@numbata Good call, def increment the version accordingly. |
|
LGTM! Release it |
|
Thank you both! 🎉 |
This PR changes how grape-swagger-entity determines if an entity field is required or not:
documentation: { required: true/false }is set, use thatiforunlessis not null or ifexpose_nilisfalse, assume the parameter is not requiredThis is consistent with how grape-entity renders fields: fields are always exposed unless
iforunlessis provided (cf. https://github.com/ruby-grape/grape-entity?tab=readme-ov-file#conditional-exposure).Closes #80.
This is probably a breaking change, so I understand if you don't want to merge this as is, but it would be nice to be able to opt into this behavior.